Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add readonly iterators and support value mutations only from non-readonly iterators #345

Conversation

fxamacker
Copy link
Member

@fxamacker fxamacker commented Sep 29, 2023

This PR:

Changes:

  • Adds readonly iterators that match current iterator API (except for the "ReadOnly" suffix added to some function names).
  • Refactors API of non-readonly iterators because register inlining will require more parameters for MapIterator.
  • Support mutations of values returned from non-readonly iterators

For readonly iterators, the caller is responsible for preventing changes to child containers during iteration because mutations of child containers are not guaranteed to persist.

For non-readonly iterators, two additional parameters are needed to update child container in parent map when child container is modified.


  • Targeted PR against main branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

This change:
- Adds ReadOnly iterators that match current iterator API
  (except for the "ReadOnly" suffix added to some function names).
- Refactors API of non-Readonly iterators because register
  inlining will require more parameters for MapIterator.

For ReadOnly iterators, the caller is responsible for preventing
changes to child containers during iteration because mutations
of child containers are not guaranteed to persist.

For non-ReadOnly iterators, two additional parameters are needed to
update child container in parent map when child container is modified.
@fxamacker fxamacker added enhancement New feature or request improvement breaking change changes to API that can break programs using Atree's API labels Sep 29, 2023
@fxamacker fxamacker self-assigned this Sep 29, 2023
@bluesign
Copy link

For readonly iterators, the caller is responsible for preventing changes to child containers during iteration because mutations of child containers are not guaranteed to persist.

Is there are non-deterministic case there?

@codecov-commenter
Copy link

codecov-commenter commented Sep 29, 2023

Codecov Report

Attention: 37 lines in your changes are missing coverage. Please review.

Comparison is base (8d02bbc) 62.59% compared to head (f671189) 62.62%.
Report is 1 commits behind head on fxamacker/inline-array-and-map.

Additional details and impacted files
@@                        Coverage Diff                         @@
##           fxamacker/inline-array-and-map     #345      +/-   ##
==================================================================
+ Coverage                           62.59%   62.62%   +0.03%     
==================================================================
  Files                                  15       15              
  Lines                               10492    10559      +67     
==================================================================
+ Hits                                 6567     6613      +46     
- Misses                               2989     3004      +15     
- Partials                              936      942       +6     
Files Coverage Δ
array.go 68.10% <80.32%> (-0.04%) ⬇️
map.go 65.30% <70.23%> (+0.01%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fxamacker
Copy link
Member Author

For readonly iterators, the caller is responsible for preventing changes to child containers during iteration because mutations of child containers are not guaranteed to persist.

Is there are non-deterministic case there?

Hi @bluesign, the readonly iterators will only be used internally in Cadence to reduce overhead. So "not guaranteed to persist" won't be a problem in this case.

…omparator-and-hashinputprovider-to-mapiterator
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

map.go Outdated Show resolved Hide resolved
map.go Outdated Show resolved Hide resolved
@fxamacker fxamacker requested a review from turbolent October 3, 2023 23:34
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

I mainly reviewed the array and map code changes, I didn't look to deep into the tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change changes to API that can break programs using Atree's API enhancement New feature or request improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants